-
Notifications
You must be signed in to change notification settings - Fork 39.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move all bash custom completions to Go #96087
Conversation
@marckhouzam: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Hi @marckhouzam. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
I've added instructions on how to test, taken from #93714 You may be interested @brianpursley @eddiezane @knight42 @WLun001 |
bf5f887
to
56a42f1
Compare
/ok-to-test |
5d701fb
to
4bbc0b1
Compare
This reverts commit 0e18f0380042b652996d795559bfb818698abec3.
Signed-off-by: Marc Khouzam <marc.khouzam@montreal.ca>
Signed-off-by: Marc Khouzam <marc.khouzam@montreal.ca>
The tests should not keep delaying this PR, so I reverted the original commit which added the tests, and I wrote a few tests in the Also, as a way to maybe to stir up some momentum, I moved to native zsh completion and removed all that extra hack-y shell code (see 77bb053).
Hopefully, we can get this merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits that can be addressed in a followup. Apologies for the delays in reviewing and thank you very much for this great effort to you @marckhouzam and @brianpursley and @phil9909 for reviews.
/lgtm
/approve
ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { | ||
var comps []string | ||
if len(args) == 0 { | ||
if strings.IndexAny(toComplete, "/.~") == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd need to support both linux and windows paths, so \\
would need to be included here as well.
// FIXME | ||
t.Skip("Should not be skipeed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, although I'd like to see this being continued in the near future.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: marckhouzam, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
😍 |
Thanks @soltysh! I will soon address your comment. Should I revert the change that moves zsh completion to being native from this PR and leave it for a different PR? |
Oh and there is still a regression with this change caused by spf13/cobra#1258 I'm still waiting for the next Cobra release: spf13/cobra#1388 |
/retest Review the full test history for this PR. Silence the bot with an |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Based on #93714, this PR finishes removing all bash completion scripting and replaces it fully with Go completions.
Advantages:
Which issue(s) this PR fixes:
Fixes kubernetes/kubectl#882
Special notes for your reviewer:
The PR is based on top of @knight42 great work of #93714. However, it has been mostly refactored when included with the rest of the PR.
The pattern that the PR follows is:
apiresources.go#CompGetResourceList(...)
get.go#CompGetResource(...)
completion.go
file is added to theutil
package with the new completion codeValidArgsFunction
field is added to every cobra command that was part of the bash__kubectl_custom_func
. EachValidArgsFunction
calls theutil
package.--namespace
,--context
,--user
,--cluster
which are global was added incmd.go
--clusterrole
which is limited to thecreate clusterrolebinding
command was added increate_clusterrolebinding.go
util
package incompletion_tests.go
Test the new completion code
Please note that for the new completion code to work, one must use the new kubectl on the command-line.
Does this PR introduce a user-facing change?:
Regression:
Custom completions don't work following concatenated short flags, e.g.,
kubectl exec -it <TAB>
.This will be fixed in the next Cobra release: spf13/cobra#1258
Only minor changes:
cp
command works slightly differently; instead of doing file completion all the time, which I found to be very verbose, I have used the same solution thathelm
implemented, which is to only show/
and./
to indicate file completion is valid, and then to trigger full file completion once the user starts typing something. You can try this by comparing:./_output/bin/kubectl cp <TAB>
and./_output/bin/kubectl cp /tmp/<TAB>
kubectl autoscale deployment --max NaN <TAB>
will trigger file completion (that is what Cobra does when there is an error in the command-line) instead of accepting the invalid flag value and continuing with completion.